controller: Prefix generated configs with pool type #218

cgwalters · 2018-12-06T22:59:57Z

This way it's easy to see at a glance where a given
config came from. Now looks like:

$ oc get machineconfig
NAME                                      AGE
00-master                                 6h
00-worker                                 6h
...
master-e92af7e0bdc0591dc11405aeabb57ccd   1m
worker-50748faa54f9cc70af13c8686c2c7a28   1m

ashcrow · 2018-12-07T14:34:02Z

/retest

ashcrow

I like it!

ashcrow · 2018-12-07T14:35:52Z

/cc @abhinavdahiya

cgwalters · 2018-12-07T16:17:52Z

Though I imagine there's some better code to do this type of thing somewhere; e.g. how deployments turn into pods prefixed with a random ID etc. I'm a kube-code novice so pointers appreciated if someone has them.

(Also, related to that, is there a reason we use a checksum rather than just a short "unique-enough ID"?)

pkg/controller/render/hash.go

abhinavdahiya · 2018-12-07T18:23:50Z

Though I imagine there's some better code to do this type of thing somewhere; e.g. how deployments turn into pods prefixed with a random ID etc. I'm a kube-code novice so pointers appreciated if someone has them.

(Also, related to that, is there a reason we use a checksum rather than just a short "unique-enough ID"?)

using checksums allows MCC to manage generated machine configs without storing that hash somewhere in annotations like deployment does for pods.

cgwalters · 2018-12-07T19:40:07Z

using checksums allows MCC to manage generated machine configs without storing that hash somewhere in annotations like deployment does for pods.

I don't understand this; what does "manage" here mean? As best I can tell nothing e.g. re-hashes a config again to compare it versus something else.

It's just about being unique right?

This way it's easy to see at a glance where a given config came from. Now looks like: ``` $ oc get machineconfig NAME AGE 00-master 6h 00-worker 6h ... master-e92af7e0bdc0591dc11405aeabb57ccd 1m worker-50748faa54f9cc70af13c8686c2c7a28 1m ```

abhinavdahiya · 2018-12-07T20:17:19Z

using checksums allows MCC to manage generated machine configs without storing that hash somewhere in annotations like deployment does for pods.

I don't understand this; what does "manage" here mean? As best I can tell nothing e.g. re-hashes a config again to compare it versus something else.

It's just about being unique right?
No

https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/render/render_controller.go#L423-L434

cgwalters · 2018-12-07T20:34:34Z

OK, understood now. Thanks!

One thing I'd note here is this is a "schema change"; it will cause new configs to be generated in existing clusters. I think that's OK because we haven't shipped yet, but now would definitely be the time to think of any other improvements to the naming scheme.

ashcrow · 2018-12-07T21:07:18Z

/test e2e-aws

kikisdeliveryservice · 2018-12-10T22:41:58Z

This would be really helpful when looking at lists of machineconfigs, but I'll hold my approval and defer to @abhinavdahiya :)

abhinavdahiya · 2018-12-12T00:21:22Z

/lgtm

openshift-ci-robot · 2018-12-12T00:21:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, cgwalters

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2018-12-12T01:51:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-12-12T03:52:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-12-12T05:52:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-12-12T07:54:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-12-12T09:54:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-12-12T11:55:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-12-12T13:56:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

ashcrow · 2018-12-12T15:05:42Z

Seeing: machine-config-server-mfplc is not healthy in this last test. However, this is not consistent.

/test e2e-aws

ashcrow · 2018-12-12T15:55:11Z

Masters didn't start/stay running.

/test e2e-aws

openshift-bot · 2018-12-12T15:58:05Z

/retest

Please review the full test history for this PR and help us cut down flakes.

crawford · 2018-12-12T17:18:43Z

https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/openshift_machine-config-operator/218/pull-ci-openshift-machine-config-operator-master-e2e-aws/679

aws_instance.bootstrap: Error launching source instance: timeout while waiting for state to become 'success' (timeout: 30s)

Same failure over here... Right around the same time. I wonder if AWS briefly fell over.

/retest

ashcrow · 2018-12-12T19:08:37Z

/test e2e-aws

It was the same error as @crawford referenced occurring again 😕.

crawford · 2018-12-12T19:30:46Z

@ashcrow Hopefully, this will be fixed by openshift/installer#882.

openshift-bot · 2018-12-12T20:00:08Z

/retest

Please review the full test history for this PR and help us cut down flakes.

ashcrow · 2018-12-12T20:36:14Z

/test e2e-aws

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 6, 2018

openshift-ci-robot requested review from abhinavdahiya and crawford December 6, 2018 23:00

ashcrow approved these changes Dec 7, 2018

View reviewed changes

abhinavdahiya reviewed Dec 7, 2018

View reviewed changes

pkg/controller/render/hash.go Outdated Show resolved Hide resolved

cgwalters force-pushed the operator-prefix-configs branch from a4a0054 to 58d6e1b Compare December 7, 2018 19:45

openshift-ci-robot assigned abhinavdahiya Dec 12, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 12, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2018

openshift-merge-robot merged commit 4dcf427 into openshift:master Dec 12, 2018

controller: Prefix generated configs with pool type #218

controller: Prefix generated configs with pool type #218

Uh oh!

Conversation

cgwalters commented Dec 6, 2018

Uh oh!

ashcrow commented Dec 7, 2018

Uh oh!

ashcrow left a comment

Choose a reason for hiding this comment

Uh oh!

ashcrow commented Dec 7, 2018

Uh oh!

cgwalters commented Dec 7, 2018

Uh oh!

Uh oh!

abhinavdahiya commented Dec 7, 2018

Uh oh!

cgwalters commented Dec 7, 2018

Uh oh!

abhinavdahiya commented Dec 7, 2018

Uh oh!

cgwalters commented Dec 7, 2018

Uh oh!

ashcrow commented Dec 7, 2018

Uh oh!

kikisdeliveryservice commented Dec 10, 2018

Uh oh!

abhinavdahiya commented Dec 12, 2018

Uh oh!

openshift-ci-robot commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

ashcrow commented Dec 12, 2018

Uh oh!

ashcrow commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

crawford commented Dec 12, 2018

Uh oh!

ashcrow commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crawford commented Dec 12, 2018

Uh oh!

openshift-bot commented Dec 12, 2018

Uh oh!

ashcrow commented Dec 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ashcrow commented Dec 12, 2018 •

edited

Loading